Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enhancement/ripple #3882

Closed
wants to merge 7 commits into from
Closed

enhancement/ripple #3882

wants to merge 7 commits into from

Conversation

nekosaur
Copy link
Member

Description

Updated ripple effect to hopefully more closely resemble material design ripple.

I was unable to find a good single solution for both rectangular and circular ripples, so I've added a circle option to the ripple directive. Someone smarter than me might have a better solution?

Motivation and Context

because

How Has This Been Tested?

with provided markup

Markup:

<template>
  <v-app id="inspire">
    <v-container fluid fill-height>
      <v-layout column wrap fill-height align-center justify-center>
        <v-flex class="d-flex no-grow align-center">
          <v-card class="ma-3" ripple height="132px" width="232px"></v-card>
          <v-card :ripple="{ circle: true }" height="200px" width="200px" class="ma-3 border"></v-card>
          <v-btn color="purple" flat>baseline</v-btn>
          <v-btn>hello</v-btn>
          <v-btn icon large>
            <v-icon>home</v-icon>
          </v-btn>
        </v-flex>
      </v-layout>
    </v-container>
  </v-app>
</template>

<script>
  export default {
    data: () => ({
    })
  }
</script>

<style lang="stylus">
  .border
    border-radius: 50% !important

  .no-grow
    flex-grow: 0

  .no-select
    user-select: none
</style>

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Improvement/refactoring (non-breaking change that doesn't add any feature but make things better)

Checklist:

  • The PR title is no longer than 64 characters.
  • The PR is submitted to the correct branch (master for bug fixes, dev for new features and breaking changes).
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have created a PR in the documentation with the necessary changes.

@codecov
Copy link

codecov bot commented Apr 19, 2018

Codecov Report

Merging #3882 into dev will decrease coverage by 0.23%.
The diff coverage is 26.82%.

Impacted file tree graph

@@            Coverage Diff            @@
##              dev   #3882      +/-   ##
=========================================
- Coverage   86.84%   86.6%   -0.24%     
=========================================
  Files         160     158       -2     
  Lines        4249    4017     -232     
  Branches     1350    1271      -79     
=========================================
- Hits         3690    3479     -211     
+ Misses        449     436      -13     
+ Partials      110     102       -8
Impacted Files Coverage Δ
src/components/VBtn/VBtn.ts 88.63% <100%> (+1.13%) ⬆️
src/mixins/routable.ts 81.25% <100%> (ø) ⬆️
src/directives/ripple.ts 35.89% <21.05%> (-5.6%) ⬇️
src/components/VMenu/mixins/menu-keyable.js 57.89% <0%> (-15.79%) ⬇️
src/mixins/validatable.js 83.01% <0%> (-7.9%) ⬇️
src/mixins/selectable.js 87.17% <0%> (-5.68%) ⬇️
src/components/VForm/VForm.js 75% <0%> (-5.65%) ⬇️
src/components/VMenu/mixins/menu-position.js 44% <0%> (-4%) ⬇️
src/components/VTextField/VTextField.js 90% <0%> (-2.53%) ⬇️
... and 65 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8dc4ef5...e70764f. Read the comment docs.

computedRipple () {
const defaultRipple = this.icon || this.fab ? { circle: true } : true
if (this.disabled) return false
else return this.ripple || defaultRipple
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if I do :ripple="false"?

@@ -15,13 +19,39 @@ declare global {
enabled?: boolean
centered?: boolean
class?: string
circle?: boolean
}
}
}

interface RippleOptions {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either export this or move it to types/*.d.ts

@@ -85,6 +85,11 @@ const VBtn = mixins(
return (!this.outline && !this.flat)
? this.addBackgroundColorClassChecks(classes)
: this.addTextColorClassChecks(classes)
},
computedRipple (): any {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This returns RippleOptions | boolean, try to avoid using any where possible.

@@ -41,7 +41,7 @@ const VBtn = mixins(
outline: Boolean,
ripple: {
type: [Boolean, Object],
default: true
default: null
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  ...
} as PropValidator<RippleOptions | boolean | null>

@@ -26,6 +26,12 @@ export default Vue.extend({
target: String
},

computed: {
computedRipple (): any {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

value: (this.ripple && !this.disabled) ? this.ripple : false
}] as any, // TODO
value: this.computedRipple
}] as any,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have left a better comment, the TODO is to remove as any when vuejs/vue#8013 is fixed

@chewy94 chewy94 added the pending team review The issue is pending a full team review label Jun 12, 2018
@KaelWD KaelWD removed the pending team review The issue is pending a full team review label Jun 16, 2018
@KaelWD
Copy link
Member

KaelWD commented Jun 16, 2018

Are we still doing this? Last time I checked it broke radios and checkboxes.

@nekosaur
Copy link
Member Author

I'll have a look

@johnleider
Copy link
Member

Closing due to inactivity.

@johnleider johnleider closed this Jul 9, 2018
@nekosaur nekosaur deleted the enh/ripple branch August 25, 2018 08:33
@lock
Copy link

lock bot commented Apr 15, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. Please direct any non-bug questions to our Discord

@lock lock bot locked as resolved and limited conversation to collaborators Apr 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants